Mega super hyper cleanup by pekkaklarck · Pull Request #956 · robotframework/SeleniumLibrary

pekkaklarck · 2017-10-13T07:10:46Z

Cleanup to docs and code here and there. See commit messages of individual commits for details. Sorry for some of the commits grewing unfortunately large.

- Remove unnecessary coupling between Element and FormElement keywords. - Enhance related error message.

- Enhance documentation of ElementKeywords (robotframework#925). - Add '.' at the end of error and log messages. - Introduce `ContextsAware.find_elements` helper. - Change default value of optional `message` from '' to None. - Use `is_noney` instead of `is_falsy` when default value is None. - General (and trivial) cleanup to code and tests.

Also added more XPath examples. This ought to cover enhancements proposed by robotframework#940.

- Doc string to `find_element`. Mainly to give type hits to IDE but higher level APIs should get more docs in general. - Introduced ElementNotFound error that `ElementFinder.find` uses. - Move helpers to ContextAware and LibraryComponent to avoid unnecessary coupling between different library componets. - Remove unnecessary helpers when `find_element(locator).method()` works as well. - Rewriter internal logic with `Wait` keywords. This includes consistently handling non-existing elements. - Make unregistering strategy that hasn't been registered an error. - Consider `<input type="file">` text element consistently. - General cleanup here and there.

aaltat · 2017-10-18T06:08:45Z

Did only read to waiting object, but have few comments

"Element Should (Not) Be Enabled" and "Wait Until Element Is Enabled" now all validate that the element is enabled and not readonly. Also removed broken element type validation from the former keywords. Fixes robotframework#958.

aaltat

Did all the changes.

aaltat · 2017-10-13T14:43:57Z

    @keyword
    def current_frame_should_not_contain(self, text, loglevel='INFO'):
-        """Verifies that current frame contains `text`.
+        """Verifies that current frame contains ``text``.


Should there be not somewhere in the sentence?

aaltat · 2017-10-18T05:54:57Z

+
+    @keyword
+    def current_frame_should_not_contain(self, text, loglevel='INFO'):
+        """Verifies that current frame contains ``text``.


I think doc string is missing text: not

aaltat · 2017-10-18T06:06:35Z

+            try:
+                if condition():
+                    return
+            except ElementNotFound as err:


I was thinking that this code could resolve the StaleElementReferenceException if we would suppress also that exception. If it does, perhaps we should look us there other exceptions that could be also suppress.

aaltat · 2017-10-18T18:56:44Z

+# limitations under the License.
+
+
+class ElementNotFound(Exception):


Having own exception is really good idea.

I think in future it is possible that we have more exceptions and because it provides better information for the user. But for programmatic, I often have to need suppress all from specific class. Therefore it might be useful to have some base error class for SeleniumLibrary exceptions. Like:

class BaseException(Exception): ROBOT_SUPPRESS_NAME = True class ElementNotFound(BaseException): pass

Then in libraries which extend SL, I could do:

try: self.sl.some_method(arg1, arg2...) expect BaseException: do_something_else(ar3)

Good idea. I'll add SeleniumLibraryException base class.

aaltat · 2017-10-18T19:14:01Z

+        using the log level specified with the optional ``loglevel``
+        argument. Valid log levels are ``DEBUG``, ``INFO`` (default),
+        ``WARN``, and ``NONE``. If the log level is ``NONE`` or below
+        the current active log level the source will not be logged.


It might be useful to mention that text is verified from the DOM tree which sometimes may differ what is seen in the page. Example the case of the text might be different in the DOM, than what is seen in the page. Other valid example if the text is split to multiple elements, example there is link in the middle of the text, then keyword can not find the whole text. Instead the search must be divided between each element.

Are you sure about the problem with finding text with elements in them? I just changed our test data to contain This is the haystack and somewhere on this page is a <em>needle</em>. and

Page Should Contain This is the haystack and somewhere on this page is a needle.

seems to work just fine.

I did have recollection that in some point it did not work, but I ques it does not hold water anymore. But the first point is a valid one and sometimes causes questions in the user group or in the issue tracker.

aaltat · 2017-10-18T19:43:32Z

-    def page_should_not_contain_link(self, locator, message='', loglevel='INFO'):
-        """Verifies image identified by `locator` is not found from current page.
+    def page_should_not_contain_link(self, locator, message=None, loglevel='INFO'):
+        """Verifies image identified by ``locator`` is not found from current page.


change "image" to "link"

aaltat · 2017-10-18T20:16:14Z


    @keyword
    def wait_until_element_is_enabled(self, locator, timeout=None, error=None):
        """Waits until element specified with `locator` is enabled.


`locator ` to ``locator``

I haven't gone through these docs yet.

No problem.

aaltat · 2017-10-18T20:16:42Z


-        Fails if `timeout` expires before the element is enabled. See
-        `introduction` for more information about `timeout` and its
+        Fails if `timeout` expires before the element is enabled. Element


`timeout` to ``timeout``

aaltat · 2017-10-18T20:18:27Z

+        See `introduction` for more information about `timeout` and its
        default value.

        `error` can be used to override the default error message.


`error` to ``error``

aaltat · 2017-10-18T20:25:01Z

-        else:
-            del self._strategies[strategy_name]
+        if strategy_name not in self._strategies:
+            raise RuntimeError("Cannot unregister the non-registered strategy '%s'."


Should there be issue about this change? It by a quick look feels backwards incompatible change.

I guess so. Submitted #961.

- User `find_elements` instead of `find_element` when possible. - Avoid `strategy=value` locator syntax in code. - Enhance error messages when radio buttons not found. - Cleanup `Choose File` test and test it with Firefox. - TableElementFinder._locator to public class variable. - Remove useless documentation.

pekkaklarck added 6 commits October 13, 2017 10:08

Cleanup.

52fd132

- Remove unnecessary coupling between Element and FormElement keywords. - Enhance related error message.

Enhance docs of different locator strategies.

4724be0

Also added more XPath examples. This ought to cover enhancements proposed by robotframework#940.

Small cleanup to logging

afcd188

Move frame related kws to own lib component

c30f825

Consistent handling of enabled status.

b78a22c

"Element Should (Not) Be Enabled" and "Wait Until Element Is Enabled" now all validate that the element is enabled and not readonly. Also removed broken element type validation from the former keywords. Fixes robotframework#958.

pekkaklarck changed the title ~~Cleanup to ElementKeywords (and elsewhere)~~ Mega super cleanup Oct 18, 2017

pekkaklarck changed the title ~~Mega super cleanup~~ Mega super hyper cleanup Oct 18, 2017

Doc that elements considered enabled cannot be read-only

eac3ecc

aaltat reviewed Oct 18, 2017

View reviewed changes

pekkaklarck mentioned this pull request Oct 19, 2017

Make it an error to unregister a locator strategy that hasn't been registered #961

Closed

pekkaklarck added 4 commits October 19, 2017 14:27

Base class for SeLib exceptions

44cba52

Doc fixes based on review comments

8106c83

Test "Page Should Contain" with text spanning multiple elements.

ee68369

aaltat approved these changes Oct 19, 2017

View reviewed changes

aaltat merged commit 4e48199 into robotframework:master Oct 19, 2017

pekkaklarck deleted the element-cleanup branch October 23, 2017 11:43

pekkaklarck restored the element-cleanup branch October 23, 2017 11:43

		# limitations under the License.


		class ElementNotFound(Exception):

Conversation

pekkaklarck commented Oct 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaltat commented Oct 18, 2017

Uh oh!

aaltat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pekkaklarck commented Oct 13, 2017 •

edited

Loading